Use application-scoped StorageCredentialCache #2022

XN137 · 2025-07-10T07:34:20Z

since StorageCredentialCache is application scoped and after 6ddd148 its constructor no longer uses the RealmContext passed into getOrCreateStorageCredentialCache we can now let all PolarisEntityManager instances share the same StorageCredentialCache.

snazy · 2025-07-10T08:18:01Z

Converted to "draft state", as it depends on another PR

snazy

Thanks a ton for this!

...src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisGenericTableCatalogTest.java

dimas-b

Nice refactoring for application-scoped data. From my POV, this PR improves control over Polaris heap usage by consolidating cached credentials in one cache object as opposed to having a (virtually unbounded) collection of caches.

dimas-b · 2025-07-10T19:28:44Z

polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java

-  public StorageCredentialCache(
-      RealmContext realmContext, PolarisConfigurationStore configurationStore) {
-    this.realmContext = realmContext;
+  public StorageCredentialCache(PolarisConfigurationStore configurationStore) {
    this.configurationStore = configurationStore;
    cache =
        Caffeine.newBuilder()


Would you mind making the max cache size (by entries) configurable? Now that the caches is shared across realms, configuring its side is more important to users, I guess.

I suppose the cache size has to be a global configuration (maybe a constructor parameter injected via CDI producers + Quarkus config).

i've added a commit for this, is this what you had in mind?
please double check the naming, module and package.

also imo this raises some questions:

a) how should the new cache config relate to FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS and FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS ?

b) what to do with the InMemoryEntityCache which seems to deal with a single realm? (and thus its max size cant be configured at the application level)

InMemoryEntityCache

I was thinking about that one, too. And I suspect it should be refactored in a similar way.

InMemoryEntityCache -> +1 to similar refactoring (better control over heap usage)

polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java

XN137 · 2025-07-11T07:56:23Z

rebased after 19f44d8 due to conflicts

XN137 · 2025-07-14T11:35:53Z

rebased after multiple conflicts, should be ready for review now

snazy · 2025-07-14T13:11:10Z

polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java

-  public StorageCredentialCache(
-      RealmContext realmContext, PolarisConfigurationStore configurationStore) {
-    this.realmContext = realmContext;
+  public StorageCredentialCache(PolarisConfigurationStore configurationStore) {
    this.configurationStore = configurationStore;
    cache =
        Caffeine.newBuilder()


InMemoryEntityCache

I was thinking about that one, too. And I suspect it should be refactored in a similar way.

snazy · 2025-07-14T13:12:10Z

...ain/java/org/apache/polaris/service/quarkus/storage/QuarkusStorageCredentialCacheConfig.java

+public interface QuarkusStorageCredentialCacheConfig extends StorageCredentialCacheConfig {
+  @WithName("max-entry-count")
+  @WithDefault("10000")
+  @Min(0)


Shouldn't this be 1?

0 is supported by caffeine, so i thought we should offer it
https://www.javadoc.io/doc/com.github.ben-manes.caffeine/caffeine/latest/com.github.benmanes.caffeine/com/github/benmanes/caffeine/cache/Caffeine.html

snazy · 2025-07-14T14:02:29Z

polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java

    cache =
        Caffeine.newBuilder()
-            .maximumSize(CACHE_MAX_NUMBER_OF_ENTRIES)
+            .maximumSize(cacheConfig.maxEntryCount())


Not related to this PR, but related to Nessie's persistence cache, discussing with Ben Manes, we opted to add those as well:

.executor(Runnable::run) .scheduler(Scheduler.systemScheduler())

Maybe not in this PR, but mentioning for posterity.

dimas-b

LGTM 👍 Thanks, @XN137 !

As the next step, I suppose it might be worth introducing a request-scoped facade to the credentials cache similar to RealmConfig.

dimas-b · 2025-07-14T16:37:17Z

...ain/java/org/apache/polaris/service/quarkus/storage/QuarkusStorageCredentialCacheConfig.java

+
+@ConfigMapping(prefix = "polaris.storage-credential-cache")
+public interface QuarkusStorageCredentialCacheConfig extends StorageCredentialCacheConfig {
+  @WithName("max-entry-count")


nit: max-entries-count?

Actually, just max-entries - it's a count, no need to repeat that ;)

max-entries LGTM

github-project-automation bot added this to Basic Kanban Board Jul 10, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jul 10, 2025

XN137 marked this pull request as ready for review July 10, 2025 08:09

snazy marked this pull request as draft July 10, 2025 08:17

snazy reviewed Jul 10, 2025

View reviewed changes

...src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisGenericTableCatalogTest.java Outdated Show resolved Hide resolved

dimas-b previously approved these changes Jul 10, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 10, 2025

XN137 dismissed dimas-b’s stale review via 7f1eaf4 July 11, 2025 07:43

XN137 force-pushed the application-scoped-StorageCredentialCache branch 2 times, most recently from 7f1eaf4 to bf5a42e Compare July 11, 2025 07:56

XN137 force-pushed the application-scoped-StorageCredentialCache branch from bf5a42e to 23c1d4e Compare July 14, 2025 10:43

XN137 added 2 commits July 14, 2025 13:17

Use application-scoped StorageCredentialCache

dcc901a

review: add StorageCredentialCacheConfig

db1c4c5

XN137 force-pushed the application-scoped-StorageCredentialCache branch from 23c1d4e to db1c4c5 Compare July 14, 2025 11:18

XN137 marked this pull request as ready for review July 14, 2025 11:35

snazy approved these changes Jul 14, 2025

View reviewed changes

snazy reviewed Jul 14, 2025

View reviewed changes

dimas-b approved these changes Jul 14, 2025

View reviewed changes

Use application-scoped StorageCredentialCache #2022

Are you sure you want to change the base?

Use application-scoped StorageCredentialCache #2022

Conversation

XN137 commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snazy commented Jul 10, 2025

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XN137 Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

XN137 commented Jul 11, 2025

Uh oh!

XN137 commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

XN137 commented Jul 10, 2025 •

edited

Loading

XN137 Jul 14, 2025 •

edited

Loading

dimas-b left a comment •

edited

Loading